Skip to content

Handle errors in parallel teardown files #2210

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Handle errors in parallel teardown files #2210

wants to merge 7 commits into from
+58 −15

Conversation

gaborcsardi
Copy link
Member

I also changed the exit status of the subprocesses to 0 (normal exit),
from 1. I didn't find any reason why it was 1, hopefully there
wasn't any. It is a normal exit, so 0 makes more senset to me (now).

Closes #1165.

@gaborcsardi
Handle errors in parallel teardown files
be38592 
I also changed the exit status of the subprocesses to 0 (normal exit),
from 1. I didn't find any reason why it was 1, hopefully there
wasn't any. It is a normal exit, so 0 makes more senset to me (now).

Closes #1165.
@claude Claude
Copy link

claude bot commented Aug 13, 2025 •
edited
Loading

Claude finished @gaborcsardi's task —— View job

Code Review

  • Read modified files to understand changes
  • Analyze test failure and its relationship to the changes
  • Identify potential bugs or issues
  • Provide code review feedback

Key Issues:

  1. Test failure appears unrelated to your changes - The test failure shows "R session closed the process connection, killed" but your teardown error collection logic should handle this properly.

  2. Exit status change from 1 to 0 - R/parallel.R:319 - This change makes sense for normal teardown exits, but ensure teardown errors still get properly surfaced through the new error collection mechanism.

  3. Worker collection fix looks correct - R/parallel.R:330 - The change from collecting connections to collecting workers is correct since you need both the connection (for polling) and the worker object (for reading messages).

The teardown error collection improvement should handle the test failure case better. The failing test might need investigation of whether the teardown error is occurring before your error collection logic can capture it.

Recommendation: Run the specific failing test in isolation to see if the error collection is working as expected, or if there's a race condition in the teardown process.

@gaborcsardi
Copy link
Member Author

Seems like this is not OK:

══ Failed tests ════════════════════════════════════════════════════════════════
── Error ('test-parallel-stdout.R:16:3'): stdout/stderr in parallel code ───────
Error in `queue_teardown(queue)`: At least one parallel worker failed to run teardown
Caused by error:
! R session closed the process connection, killed
Backtrace:
     ▆
  1. ├─utils::capture.output(...) at test-parallel-stdout.R:16:3
  2. │ └─base::withVisible(...elt(i))
  3. ├─base::suppressMessages(...)
  4. │ └─base::withCallingHandlers(...)
  5. └─testthat::test_local(test_path("test-parallel", "stdout"), reporter = "progress")
  6.   └─testthat::test_dir(...) at testthat/R/test-package.R:73:3
  7.     └─testthat:::test_files(...) at testthat/R/test-files.R:88:3
  8.       └─testthat:::test_files_parallel(...) at testthat/R/test-files.R:165:5
  9.         ├─withr (local) `<fn>`() at testthat/R/parallel.R:67:3
 10.         └─testthat:::queue_teardown(queue) at testthat/R/parallel.R:54:3
 11.           └─cli::cli_abort(...) at testthat/R/parallel.R:388:5
 12.             └─rlang::abort(...)

@gaborcsardi
Copy link
Member Author

Another kind of failure:

══ Failed tests ════════════════════════════════════════════════════════════════
── Failure ('test-parallel-crash.R:22:3'): crash ───────────────────────────────
`err` inherits from 'rlang_error'/'error'/'condition' not 'testthat_process_error'.
── Failure ('test-parallel-crash.R:23:3'): crash ───────────────────────────────
err$test_file (`actual`) is not equal to "test-crash-3.R" (`expected`).

`actual` is NULL
`expected` is a character vector ('test-crash-3.R')

[ FAIL 2 | WARN 0 | SKIP 4 | PASS 1221 ]

@gaborcsardi
Merge branch 'main' into fix/parallel-teardown
d801d65
@gaborcsardi
Only call teardown on idle workers
f7d6b59 
It is an error to call it on busy ones. This might happen
on an interrupt.
@gaborcsardi
Need to tryCatch() worker cleanup
a5b90c1 
In case the process has terminated already.
@gaborcsardi
Do not call quit() in worker teardown
337139e 
Because it creates a race condition and the output
of the teardown function might be lost.

This means that covr probably does not work for these
workers, because there is no time for it to write out
the coverage counters.
@gaborcsardi
Improve parallel teardown
92acd3d 
Close the input connection first, and then give the workers
some time to quit. This helps covr to write out the coverage
counters.
@gaborcsardi
Copy link
Member Author

This should be good now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Handle errors in parallel teardown files

1 participant

@gaborcsardi